Skip to content

Include canonical issue IDs in duplicate close intents - #73078

Merged
pelikhan merged 4 commits into
mainfrom
copilot/send-duplicate-issue-id
Oct 30, 2026
Merged

pelikhan merged 4 commits into
mainfrom
copilot/send-duplicate-issue-id

Conversation

Copilot AI commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Duplicate close intents omitted the required duplicate_issue_id, causing HTTP 422 instead of creating a pending suggestion for maintainer review.

  • Request payload: Resolve duplicate_of to the canonical issue’s REST database ID and include it as duplicate_issue_id.
  • Safety: Preserve repository allowlists and self-reference checks. Canonical lookup failures propagate without triggering a legacy close.
  • Regression coverage: Cover supported reference formats, cross-repository resolution, lookup failures, and ineligible references.

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix duplicate issue ID handling in close-issue intents Include canonical issue IDs in duplicate close intents Oct 30, 2026
Copilot AI requested a review from pelikhan October 30, 2026 01:32
@pelikhan
pelikhan marked this pull request as ready for review October 30, 2026 05:12
Copilot AI auto review requested due to automatic review settings October 30, 2026 05:12
@github-actions

github-actions Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer

@github-actions

github-actions Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

✅ Test Quality Sentinel completed test quality analysis.

Test Quality Sentinel

Outcome: reported_no_action

The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible.

Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.

Generated from Test Quality Sentinel · haiku45 · ~6 AIC · ⌖ ~8 AIC · ◇ ~4 AIC · ⊞ 8.9K · ◷

🧪 Test quality analysis by Test Quality Sentinel

@github-actions

github-actions Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.

Design Decision Gate 🏗️

Outcome: reported_no_action

The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible.

Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.

Generated from Design Decision Gate 🏗️ · opus55 · ~12 AIC · ⌖ ~38 AIC · ◇ ~3 AIC · ⊞ 1.9K · ◷

🏗️ ADR gate enforced by Design Decision Gate 🏗️

@github-actions

github-actions Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

✅ Ponytail Reviewer completed successfully!

Ponytail Reviewer

Outcome: reported_no_action

The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible.

Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.

Generated from Ponytail Reviewer · gpt56 · ~10 AIC · ⌖ ~6 AIC · ⊞ 14.8K · ◷

Generated by Ponytail Reviewer for #73078

@github-actions

github-actions Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

✅ PR Code Quality Reviewer completed the code quality review.

🔎 Code quality review by PR Code Quality Reviewer

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The case-sensitive self-duplicate guard allows mixed-case references to the same repository and issue, resulting in an invalid GitHub request.

1 open issue
What changed in this PR

This PR addresses #73054 by resolving canonical issue references to REST database IDs for duplicate-close intents, allowing the API to create pending suggestions for maintainer review.

Changes:

  • Include duplicate_issue_id in eligible duplicate-close intent requests.
  • Propagate canonical lookup failures without falling back to a legacy close.
  • Add regression coverage for reference formats, cross-repository resolution, lookup failures, and ineligible references.
File Description
actions/​setup/​js/​close_issue.test.cjs Adds canonical-ID resolution and failure-path coverage.
actions/​setup/​js/​close_issue.cjs Resolves eligible canonical references and includes their IDs in intent payloads.

Review effort: Balanced (auto)

Comment thread actions/setup/js/close_issue.cjs Outdated
const closePromise = closeIssue(github, owner, repo, entityNumber, stateReason, intentMetadata, issueIntentEnabled);
const stateReasonUpper = stateReason.toUpperCase();
const parsed = stateReasonUpper === "DUPLICATE" ? parseDuplicateOf(item.duplicate_of, owner, repo) : null;
const canonicalIssue = parsed && validateRepo(`${parsed.owner}/${parsed.repo}`, defaultTargetRepo, allowedRepos).valid && !(parsed.owner === owner && parsed.repo === repo && parsed.issueNumber === entityNumber) ? parsed : undefined;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the self-reference guard to compare owner and repository case-insensitively, with a mixed-case regression test in close_issue.test.cjs. Commit e4e5036.

@github-actions

Copy link
Copy Markdown
Contributor

Comment Memory

Peek at saved memory (pr-code-quality-reviewer)
reviewed_at: 2026-10-30T05:18:00Z
review_event: REQUEST_CHANGES
top_themes:
  - invalid duplicate references still emit intent PATCHes without duplicate_issue_id
  - case-sensitive self-reference guard
files_reviewed:
  - actions/setup/js/close_issue.cjs
  - actions/setup/js/close_issue.test.cjs
comment_count: 2

Note

This comment is managed by comment memory.

Expand the saved memory block to view or edit the persistent context for this thread.
Edit only the text inside the backtick fences; workflow metadata and the footer are regenerated automatically.

Learn more about comment memory

🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~3 AIC · ⌖ ~1 AIC · ⊞ 1.6K · ◷
Comment /review to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes

The new canonical-ID path still permits invalid/self duplicate references to reach the issue-intent PATCH without duplicate_issue_id, preserving the 422 failure this PR is intended to fix. The self-reference comparison also needs repository-name normalization for wildcard targets.

Details
  • Ineligible or self-referential duplicate_of values become undefined canonical targets, but still emit a duplicate intent without the required ID.
  • Mixed-case repository references can bypass the self-reference guard because comparisons are case-sensitive.

🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~3 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again

Comment thread actions/setup/js/close_issue.cjs Outdated
const closePromise = closeIssue(github, owner, repo, entityNumber, stateReason, intentMetadata, issueIntentEnabled);
const stateReasonUpper = stateReason.toUpperCase();
const parsed = stateReasonUpper === "DUPLICATE" ? parseDuplicateOf(item.duplicate_of, owner, repo) : null;
const canonicalIssue = parsed && validateRepo(`${parsed.owner}/${parsed.repo}`, defaultTargetRepo, allowedRepos).valid && !(parsed.owner === owner && parsed.repo === repo && parsed.issueNumber === entityNumber) ? parsed : undefined;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still sends a duplicate close intent without the required duplicate_issue_id whenever duplicate_of is invalid, disallowed, or self-referential, so those cases continue to fail with HTTP 422 instead of closing successfully or failing before mutation.

💡 Reject or explicitly downgrade these cases

canonicalIssue becomes undefined for these inputs, but closeIssue does not know whether that means no reference was supplied or an ineligible reference was supplied. It therefore emits the same state_reason: duplicate intent payload without an ID. Please validate an ineligible/self reference before calling closeIssue (or deliberately choose the legacy close path), and add coverage for the resulting behavior rather than asserting the known-invalid PATCH shape.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate intents now reject invalid, disallowed, and self-referential targets before any mutation; tests cover those paths. Commit e4e5036.

Comment thread actions/setup/js/close_issue.cjs Outdated
const closePromise = closeIssue(github, owner, repo, entityNumber, stateReason, intentMetadata, issueIntentEnabled);
const stateReasonUpper = stateReason.toUpperCase();
const parsed = stateReasonUpper === "DUPLICATE" ? parseDuplicateOf(item.duplicate_of, owner, repo) : null;
const canonicalIssue = parsed && validateRepo(`${parsed.owner}/${parsed.repo}`, defaultTargetRepo, allowedRepos).valid && !(parsed.owner === owner && parsed.repo === repo && parsed.issueNumber === entityNumber) ? parsed : undefined;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The self-reference guard is case-sensitive even though GitHub repository names are case-insensitive, so with a wildcard target policy a reference such as Owner/Repo#100 can resolve to the issue being closed and be sent as its own duplicate_issue_id.

💡 Normalize repository names before the self check

Compare parsed.owner and parsed.repo to owner and repo after lowercasing (or use a canonical repository identity), and add a mixed-case self-reference regression test. The same guard is duplicated in the native-marking branch below, so both paths need the normalized comparison.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shared eligibility check now rejects mixed-case self-references for both intent payloads and native duplicate marking; added regression coverage. Commit e4e5036.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skills-Based Review 🧠

Applied /codebase-design — solid fix with good regression coverage (parameterised tests for reference formats, lookup failures, and ineligible references), but two structural concerns worth addressing before merge.

📋 Key Themes & Highlights

Key Themes

  • Redundant API call: closeIssue fetches the canonical issue for its REST id, then markIssueAsDuplicate fetches the same issue again moments later for its node_id. Two GET calls per duplicate-close is avoidable and adds latency/rate-limit pressure.
  • Duplicated eligibility logic: the "is this a valid, allowed, non-self canonical reference" check is computed once inline for canonicalIssue (line 370) and then re-derived with separate if checks a few lines later for the native markAsDuplicate call. Two independent implementations of the same policy risk drifting apart silently.

Positive Highlights

  • ✅ Fail-closed behavior on canonical lookup failure (no legacy fallback, no partial mutation) is correctly implemented and well tested — exactly matches the PR's stated safety goal.
  • ✅ Good breadth of regression tests: multiple duplicate_of reference formats, cross-repo resolution, 403/404/missing-ID lookup failures, and ineligible-reference cases are all covered with clear parameterised tests.
  • ✅ Reuses the existing validateRepo/allowlist policy rather than introducing a parallel authorization path.

Neither issue is blocking, but consolidating the duplicate fetch and the eligibility check into shared helpers would reduce future maintenance risk. Posted as inline comments with concrete suggestions.

@copilot please address the review comments above.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · ~64 AIC · ⌖ ~16 AIC · ⊞ 10.2K
Comment /matt to run again

Comment thread actions/setup/js/close_issue.cjs Outdated
owner: canonicalIssue.owner,
repo: canonicalIssue.repo,
issue_number: canonicalIssue.issueNumber,
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/codebase-design] Fetching the canonical issue here (for its REST id) duplicates the GET that markIssueAsDuplicate makes a few lines later for the same issue's node_id, costing an extra API round trip (and an extra rate-limit hit) on every duplicate-close path.

💡 Suggested consolidation

Consider having closeIssue return (or accept a shared fetcher/cache for) the canonical issue data, or restructure so the single github.rest.issues.get call feeding duplicate_issue_id also supplies node_id to markIssueAsDuplicate, avoiding two separate fetches of the same resource per close.

@copilot please address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The canonical REST response used for duplicate_issue_id is now reused for the native GraphQL relationship, avoiding a second GET; the test asserts this. Commit e4e5036.

Comment thread actions/setup/js/close_issue.cjs Outdated
const closePromise = closeIssue(github, owner, repo, entityNumber, stateReason, intentMetadata, issueIntentEnabled);
const stateReasonUpper = stateReason.toUpperCase();
const parsed = stateReasonUpper === "DUPLICATE" ? parseDuplicateOf(item.duplicate_of, owner, repo) : null;
const canonicalIssue = parsed && validateRepo(`${parsed.owner}/${parsed.repo}`, defaultTargetRepo, allowedRepos).valid && !(parsed.owner === owner && parsed.repo === repo && parsed.issueNumber === entityNumber) ? parsed : undefined;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/codebase-design] The eligibility check for canonicalIssue (parsed + repo-allowed + not-self-reference) is re-derived independently in the if (item.duplicate_of ...) block below (lines ~379-391). Duplicated validation logic in two places risks silent divergence if one path is updated without the other.

💡 Suggested refactor

Extract a single helper, e.g. resolveEligibleCanonicalIssue(parsed, owner, repo, entityNumber, defaultTargetRepo, allowedRepos), returning { eligible, canonicalIssue, reason }. Use its result both to decide canonicalIssue for closeIssue(...) and to drive the warnings/skip logic in the native-duplicate-marking block, so there's one source of truth for "is this a legitimate, allowed, non-self canonical reference".

@copilot please address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added resolveEligibleCanonicalIssue as the single source for parsing/allowlist/self-reference eligibility used by both intent and native-marking paths. Commit e4e5036.

@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot run pr-finisher skill

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

@copilot run pr-finisher skill

Ran the PR finisher checks and pushed the review fixes in e4e5036. Local formatting, lint, typecheck, and 91 impacted JavaScript tests pass; CI needs a maintainer re-trigger after this push.

Comment thread actions/setup/js/close_issue.cjs Outdated
});
let canonicalData = canonicalIssueData;
if (!canonicalData) {
const response = await github.rest.issues.get({

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot use withRetry to harden call

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapped the canonical lookup in withRetry using the rate-limit retry configuration. Commit: 6ba8dc4e.

Comment thread actions/setup/js/close_issue.cjs Outdated
throw new Error(`${ERR_VALIDATION}: duplicate close intent requires a valid, allowed, non-self duplicate_of reference`);
}
if (canonicalIssue) {
const { data } = await github.rest.issues.get({

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot use withRetry

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapped this canonical lookup in withRetry using the rate-limit retry configuration. Commit: 6ba8dc4e.

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI requested a review from pelikhan October 30, 2026 12:55
@pelikhan
pelikhan merged commit 9afc034 into main Oct 30, 2026
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Send duplicate issue ID with close-issue intents

3 participants